Skip to content
This repository was archived by the owner on Nov 22, 2024. It is now read-only.

feat(socket-engine): add providers parameter#1072

Merged
CaerusKaru merged 3 commits intoangular:masterfrom
someApprentice:patch-1
Sep 29, 2018
Merged

feat(socket-engine): add providers parameter#1072
CaerusKaru merged 3 commits intoangular:masterfrom
someApprentice:patch-1

Conversation

@someApprentice
Copy link
Copy Markdown
Contributor

@someApprentice someApprentice commented Sep 24, 2018

No description provided.

@googlebot
Copy link
Copy Markdown

googlebot commented Sep 24, 2018

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.

What to do if you already signed the CLA

Individual signers
Corporate signers

1 similar comment
@googlebot
Copy link
Copy Markdown

googlebot commented Sep 24, 2018

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.

What to do if you already signed the CLA

Individual signers
Corporate signers

@someApprentice someApprentice mentioned this pull request Sep 24, 2018
Closed
@CaerusKaru CaerusKaru self-assigned this Sep 24, 2018
@CaerusKaru CaerusKaru added action: cleanup target: minor target: minor This PR is targeted for the next minor release comp: socket-engine labels Sep 24, 2018
@CaerusKaru CaerusKaru added this to the 6.0 milestone Sep 24, 2018
@CaerusKaru CaerusKaru changed the title Added providers injector feat(socket-engine): add providers parameter Sep 24, 2018
@CaerusKaru
Copy link
Copy Markdown
Member

CaerusKaru commented Sep 24, 2018

Please sign the CLA, fix the build errors, and add a small unit test. Other than that, great work!

@someApprentice
Copy link
Copy Markdown
Contributor Author

someApprentice commented Sep 24, 2018

Please sign the CLA, fix the build errors, and add a small unit test. Other than that, great work!

Affirmative! I will do that tomorrow, since time in my timezone is quite late.

@someApprentice
Copy link
Copy Markdown
Contributor Author

someApprentice commented Sep 26, 2018

@CaerusKaru

Im pretty new at JavaScript and Angular itself and I have some issues with a building application.

After npm install and /universal$ bazel build -- :all I did bazel run @yarn//:yarn and /universal$ ./scripts/build-modules-dist.sh as it was on CircleCI, and I got this error:

ERROR: /universal/modules/common/BUILD.bazel:19:1: ng_package: Rollup //modules/common:npm_package failed (Exit 1)
[!] Error: 'RenderOptions' is not exported by ../../../../../execroot/nguniversal/bazel-out/k8-fastbuild/bin/modules/common/npm_package.es6/modules/common/engine/src/index.js
https://github.com/rollup/rollup/wiki/Troubleshooting#name-is-not-exported-by-module
../../../../../execroot/nguniversal/bazel-out/k8-fastbuild/bin/modules/common/npm_package.es6/modules/common/engine/private_api.js (12:67)

Followed by suggested link I checked these files and as expected I didn't found exported RenderOptions in ...src/index.js:

export { } from "./interfaces";

...src/interfaces.js:

export function RenderOptions() { }
function RenderOptions_tsickle_Closure_declarations() {
    /** @type {?} */
    RenderOptions.prototype.bootstrap;
    /** @type {?|undefined} */
    RenderOptions.prototype.providers;
    /** @type {?|undefined} */
    RenderOptions.prototype.url;
    /** @type {?|undefined} */
    RenderOptions.prototype.document;
    /** @type {?|undefined} */
    RenderOptions.prototype.documentFilePath;
}

Can you give some tips what am I doing wrong? Should I simple add this dependency to the export?

@someApprentice someApprentice mentioned this pull request Sep 27, 2018
Closed
@someApprentice
Copy link
Copy Markdown
Contributor Author

someApprentice commented Sep 28, 2018

Can you give some tips what am I doing wrong? Should I simple add this dependency to the export?

I fixed this issue simple removed RenderOptions as ɵRenderOptions dependency from universal/modules/common/engine/private_api.ts, rebuild application (got error about lacking interface), returned this dependency back and rebuild application again. That time error didn't happen. Not sure what caused that behavior at the first time. Hope that helps someone in the future.

@someApprentice
Copy link
Copy Markdown
Contributor Author

someApprentice commented Sep 29, 2018

I signed it!

@googlebot
Copy link
Copy Markdown

googlebot commented Sep 29, 2018

CLAs look good, thanks!

@googlebot
Copy link
Copy Markdown

googlebot commented Sep 29, 2018

CLAs look good, thanks!

@someApprentice
Copy link
Copy Markdown
Contributor Author

someApprentice commented Sep 29, 2018 •
edited
Loading

ci/circleci: integration_test — Your tests failed on CircleCI

error "/home/circleci/ng/node_modules/@angular/animations" doesn't exist.

Not sure what causing that error on CircleCi. On my local machine the test is passed. Please check out your CircleCi environment and give me admonition if I though made a mistake.

universal$ ./integration/run_tests.sh
##################################
scripts/build-modules-dist.sh:
  building @nguniversal/* npm packages
##################################
WARNING: Switched off --watchfs again... temporarily falling back to manually checking files for changes
INFO: Analysed 7 targets (0 packages loaded).
INFO: Found 7 targets...
INFO: Elapsed time: 5.176s, Critical Path: 0.57s
INFO: 0 processes.
INFO: Build completed successfully, 1 total action
# Copy artifacts to /home/user/Downloads/universal/scripts/../dist/modules-dist/aspnetcore-engine
# Copy artifacts to /home/user/Downloads/universal/scripts/../dist/modules-dist/common
# Copy artifacts to /home/user/Downloads/universal/scripts/../dist/modules-dist/express-engine
# Copy artifacts to /home/user/Downloads/universal/scripts/../dist/modules-dist/hapi-engine
# Copy artifacts to /home/user/Downloads/universal/scripts/../dist/modules-dist/module-map-ngfactory-loader
# Copy artifacts to /home/user/Downloads/universal/scripts/../dist/modules-dist/socket-engine
#################################
Running integration test common
#################################
yarn install v1.10.1
info No lockfile found.
[1/4] Resolving packages...
warning babel-preset-es2015@6.24.1: 🙌  Thanks for using Babel: we recommend using babel-preset-env now: please read babeljs.io/env to update! 
[2/4] Fetching packages...
info fsevents@1.2.4: The platform "linux" is incompatible with this module.
info "fsevents@1.2.4" is an optional dependency and failed compatibility check. Excluding it from installation.
[3/4] Linking dependencies...
[4/4] Building fresh packages...
success Saved lockfile.
$ webdriver-manager update --gecko false --standalone false $CHROMEDRIVER_VERSION_ARG
[19:27:07] I/file_manager - creating folder /home/user/Downloads/universal/integration/common/node_modules/webdriver-manager/selenium
[19:27:07] I/config_source - curl -o/home/user/Downloads/universal/integration/common/node_modules/webdriver-manager/selenium/chrome-response.xml https://chromedriver.storage.googleapis.com/
[19:27:09] I/downloader - curl -o/home/user/Downloads/universal/integration/common/node_modules/webdriver-manager/selenium/chromedriver_2.42.zip https://chromedriver.storage.googleapis.com/2.42/chromedriver_linux64.zip
[19:27:10] I/update - chromedriver: unzipping chromedriver_2.42.zip
[19:27:10] I/update - chromedriver: setting permissions to 0755 for /home/user/Downloads/universal/integration/common/node_modules/webdriver-manager/selenium/chromedriver_2.42
Done in 69.82s.

...

CaerusKaru
CaerusKaru approved these changes Sep 29, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@CaerusKaru CaerusKaru left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@CaerusKaru CaerusKaru added PR state: LGTM action: merge PR author is ready for this to merge and removed action: cleanup labels Sep 29, 2018
@CaerusKaru
Copy link
Copy Markdown
Member

CaerusKaru commented Sep 29, 2018

The integration test job can be pretty flaky. I’ve been meaning to fix it, but in the meantime I’ve restarted the job (which usually works).

@CaerusKaru CaerusKaru merged commit c16860c into angular:master Sep 29, 2018
@CaerusKaru
Copy link
Copy Markdown
Member

CaerusKaru commented Sep 29, 2018

@someApprentice Thank you again for putting this together, you did a great job!

@angular-automatic-lock-bot
Copy link
Copy Markdown

angular-automatic-lock-bot Bot commented Sep 6, 2019

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot Bot locked and limited conversation to collaborators Sep 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@CaerusKaru CaerusKaru CaerusKaru approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@CaerusKaru CaerusKaru

Labels

action: merge PR author is ready for this to merge target: minor target: minor This PR is targeted for the next minor release

Projects

None yet

Milestone

6.0

Development

Successfully merging this pull request may close these issues.

3 participants

@someApprentice @googlebot @CaerusKaru